Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Geofences with Google Location API #914

Closed
wants to merge 4 commits into from

Conversation

CasualTriangle
Copy link

@CasualTriangle CasualTriangle commented Jun 3, 2018

Hello there,
I have started to implement geofences in the app.
the idear was to let the user define a new switch-item in the App and then change the state (through a REST Api call) on the OP2 Server when the Phone gets into or out of the geofence.

Currently implemented:

  • A new page (selectable under the notifications tab) for geofences
  • Adding and removing Geofences in that page
  • A basic dialog to define parameters for the geofence (Location is currently hardcoded, but will change that so you can pick from a google maps instance)

Not implemented yet / up for debate:

  • the call to the REST Api (not sure how that works yet)
  • picking the location with a Map
  • as this is a PlayStore only feature, have the foss version still work.
  • the automatic re-registering of the fences when the phone reboots
  • having the location stored on the server somehow, so that other phones connected to the same OP2 Server can choose Locations from a list instead of defining them again ?

Thanks,
Nils

@kaikreuzer
Copy link
Member

Thanks for working on this!
Just to make sure that there are no duplicate efforts: Are you aware of #343 and #355?

Also note that @Tony-hu is currently working on #880, which imho is pretty much related. What I have discussed with him there is that it would be great to have some concepts of "person x being at location y" and the apps providing this information. Saying "person x is near beacon y" or "person x is in geofence z" is pretty much the same - the logic of what should happen in such a case should imho definitely be on the server side (as rules) and not in the app.

@CasualTriangle
Copy link
Author

Yes, I am aware of the other PRs.
Regarding #355, I'm not sure though if there is still someone working on it, as there wasn't any commits since august last year and the app changed a whole lot since then so its probably easier to rewrite it anyways.

@Tony080
Copy link

Tony080 commented Jun 3, 2018

Hello @CasualTriangle , we are doing the features with something in common with #880 . Perhaps we can make further discussion in the future and I doubt if we will share the same configuration activity in the future.

the call to the REST Api (not sure how that works yet)

I have the similar situation to deal with the question: how to make the connection between detecting location changes and controlling thing(s)? My solution is, as @kaikreuzer said, leave the controlling authority to the server and all the cellphone did are just detecting and reporting state changes. Once the server gets the state changes, it is the server's responsibility to do some stuff based on existing rules.

@CasualTriangle
Copy link
Author

I have implemented a basic activity with a recyclerview that lets the user add and define openhab-switch-items (the name and label).
Now we could add a 3rd option for every item that triggers the state change (whatever that might be... in our case a beacon is in range or the phone enters a geofence). Then when choosing "BLE Beacon" as the "trigger" for the item would open your settings activity and the user can choose the Beacon. Or he chooses "Geofence" and another settings activity opens.
i think that would make setting up switch-items easier and more consistent for the user (and we also could share the "sending the new state"-part of the code)

@Tony080
Copy link

Tony080 commented Jun 3, 2018

I have implemented a basic activity with a recyclerview that lets the user add and define openhab-switch-items (the name and label).

Cool! I almost did the same thing also using RecyclerView and SwipeRefreshLayout to display all beacon scan results.
I like your ideas about sharing the activity. Let's keep in mind and see what we can do once we are there.

Copy link
Contributor

@maniac103 maniac103 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments after a quick code inspection. Didn't try it yet; will look at it more in detail after 'WIP' status is resolved.

@@ -13,6 +16,13 @@
</intent-filter>
</receiver>

<activity android:name="org.openhab.habdroid.ui.OpenHABGeofenceFragment"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fragment is not an activity. What exactly is the reason for adding this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had the fragment initaly as a activity... will delete that now that its a fragment


public static boolean isGeofenceFeatureAvailable() {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks unneeded/misplaced?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this function will return false is the foss flavor.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes this is for the foss

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the same method exists in GeofencingHelper, and is placed correctly there. The class GeofencingService doesn't even exist for foss.

return true;
}

public static class GeofenceNameNotUnique extends IllegalArgumentException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GeofenceNameNotUniqueException

if (loadOpenHABGeofencesFromMemory(activity).contains(newOpenHABGeofence)) {
throw new GeofenceNameNotUnique();//Geofence name already registered
}
List<OpenHABGeofence> newOpenHABGeofences = new ArrayList<>(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not only apply here, but also elsewhere: please drop 'OpenHAB' from variable and class names. Yes, other places in the code base have it like that, but those places are going to be changed accordingly ;-)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok will do that, did that on the geofences to differenciate the OpenHAB instance form the google one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe name it StoredGeofence or AssignedGeofence or ItemGeofence or something among those lines?

* @param geofencesForRemoval
*/
public static void removeGeofences(Activity activity, List<OpenHABGeofence> geofencesForRemoval) {
if(geofencesForRemoval.isEmpty()) {return;}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also doesn't apply here only, but in other places as well: Please try to follow the code style - we use the AOSP code style: https://source.android.com/setup/contribute/code-style

In this particular place the corrected version would be

if (geofencesForRemoval.isEmpty()) {
    return;
}

public static final String SUBSCREEN_REMOTE_CONNECTION = "default_openhab_remote_connection";
public static final String SUBSCREEN_SSL_SETTINGS = "default_openhab_ssl";

public static final int REQUEST_LOCATION_REQUEST_CODE = 42;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably define this in OpenHABMainActivity

* @param longitude the longitude
* @return
*/
public static String coordinatesStringFromValues(double latitude,double longitude) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too specific to live here; I don't think there's a realistic chance of reusing this.

@@ -0,0 +1,44 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These layout files should be moved to the full package.

<string name="geofence_coordinates">Coord.</string>
<string name="geofence_radius">Radius</string>
<string name="geofence_dialog_create">Create</string>
<string name="geofence_dialog_cancel">Cancel</string>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those strings are - AFAICT - full package specific, why not move them to there?

<string name="geofence_dialog_new">New Geofence</string>
<string name="geofence_label">Label</string>
<string name="geofence_name">Name</string>
<string name="geofence_coordinates">Coord.</string>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why shorten this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks bad on my phone if written out

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In abbreviated form it'll likely look bad on someone else's phone. This sounds like it could/should be solved by a better placement of controls instead. Can you provide a screenshot of the dialog?

@mueller-ma
Copy link
Member

I agree that geofences and beacons should have a common navigation drawer entry. Opening that should show all currently configured fences and beacons. A click on the "add" fab could expand it to allow adding geofences and beacons: https://material.io/design/assets/1fohUHLBjjut8g3UsgGGHHBV6PBboy8EK/fab-transitions-speeddial-do-labels.png. Those "adding" screens can be differ between the two things.

I'm not sure if it's the best way to create the switch item during the configuration. Having a searchable list with all items that are defined server side seems a better option to me.

When a user defines a lot of fences or beacons it might be easier for him to check for one text item that has the name of the fence or beacon he is near as state or null/undefined if he isn't near a beacon and outside all fences. This should be optional.

@Tony080
Copy link

Tony080 commented Jun 5, 2018

A click on the "add" fab could expand it to allow adding geofences and beacons:

I love this kind of design. Perhaps CasualTriangle and I could share the same drawer entry activity. And once the user clicks on the fab, the app will open specified fragment.

I'm not sure if it's the best way to create the switch item during the configuration. Having a searchable list with all items that are defined server side seems a better option to me.

I'm wondering that if each phone should be treated as a location item(possibly 2 items for both beacon and geo-fence, not sure). It can have some states like "kitchen", "living room", "absent" and etc. And the app not only reports for the state but also adds new states if more beacons/geofences are introduced. So, you know, in this situation, the item can hold more states than a switch does.

When a user defines a lot of fences or beacons it might be easier for him to check for one text item that has the name of the fence or beacon he is near as state or null/undefined if he isn't near a beacon and outside all fences. This should be optional.

Agreed. The app only shows the nearest beacon/geo-fence(of course this should be the same as the current state) on the entry activity. But leaves the rest stuff in the configuration fragment. Right?

@mueller-ma mueller-ma added the enhancement Indicates new feature requests label Jun 5, 2018
@kaikreuzer
Copy link
Member

I'm not sure if it's the best way to create the switch item during the configuration.

As mentioned above already, I am sure that it is NOT the best way ;-)

it would be great to have some concepts of "person x being at location y"

As we are just about to create an initial ESH tag library, my suggestion would be to align this what we need here:

We could use Location items for persons and phones should then be assigned to (exactly) one of these items. If they were tagged with object:person, the app could do a simple rest request for all items with such a tag and ask the user to select one (this would need to be done somewhere in the main settings).

All our rooms/homes/offices/otherplaces are usually already modelled as Group items - if those were tagged with object:place (or whatever identifies an item to be a place), this list could be requested by the app and beacons and geofences could be assigned to those.

Whenever the phone is near a beacon or within a geofence, it could simply send the name of the associated place-item to the person-item.

To be able to do this, Location items should not only allow geo-coordinates as a state, but also place-item names, but this is a change that was anyhow considered when introducing Location items.

One assumption that is implicitly made here is that we won't have any intersections between places, but only containments - i.e. all beacons that are in my rooms at home can be considered to be within my "home geofence" and my "office geofence" is not intersecting with my "home-geofence". I would think that as we want to locate a person with that feature, this assumption is perfectly fine, though - but feel free to disagree, if I miss anything.

Wdyt: Does that concept make sense?

@kaikreuzer
Copy link
Member

I'd actually also like to reference eclipse-archived/smarthome#582, which was created in 2015 "to better support use cases like geo-fencing, iBeacons / BLE tags, wearables and rules with user-context" - maybe we can finally get there :-)

@kaikreuzer
Copy link
Member

@CasualTriangle Any feedback from your end on my proposal?
I'd love to see progress here (and especially have it fully aligned with #880). Would be great if you, @mueller-ma and @Tony-hu could discuss a plan how to best align and progress those two PRs. I can help wrt required features on the server side (like requesting a list of places or persons through the REST API). Wdyt?

@CasualTriangle
Copy link
Author

@kaikreuzer after looking through the proposals i started to like the idear of "pinning" location tags on a items and then having items for every person.
That would mean that the only attribute the user has to set up for every geofence is a place-item ? (then there should be some system in place that lets the user easily define places in OH2; As mentioned earlier by picking them on a map for example)

One assumption [...] we won't have any intersections between places

If i understood the concept right then even that would be possible, wouldn't it ?
e.g. if you define one for your town and one for your house:
when entering the fences the app would just put both "town" and "house" in the location tag of the item.
Item name:person1_with_habdroid object:person location:town,house
and leaving just the house would remove just the "house" from the tag.

@kaikreuzer
Copy link
Member

That would mean that the only attribute the user has to set up for every geofence is a place-item ?

Yes, he would have to create those items, but it is likely that they even already exist - e.g. when using the HomeBuilder, it creates an item

Group   Home   "Our Home"   <house>

which would be the perfect target for a geofence.

then there should be some system in place that lets the user easily define places in OH2; As mentioned earlier by picking them on a map for example

Yes, this should be a screen like @mueller-ma described in #914 (comment):

Opening that should show all currently configured fences and beacons. A click on the "add" fab could expand it to allow adding geofences and beacons. Those "adding" screens can be differ between the two things.

when entering the fences the app would just put both "town" and "house" in the location tag of the item

No, persons would be a "Location" item, i.e. the item state defines their location (as this is something dynamic and not statically defined). And there can always only be one state, i.e. one location at a time (which somewhat makes sense). For house+town it would still be easy, because house is within town, so it is simply a more precise location (and from being "in house" you can implicitely derive that you are also "in town".

@Tony080
Copy link

Tony080 commented Jun 13, 2018

I have written an activity for displaying the binding pairs of beacon - frame. Though it's still an ugly debugging interface, I'm wondering if this activity can be the foundation of configured geo-fences and beacons common activity.

It's displaying nearest beacon with distance as the title of the activity. And all the paired beacon name - frame has been displayed.
And there is a FAB to allow you to introduce more beacons. It's directly open the beacon configuration activity if clicked for now. But I tend to modify it later to act like the FAB speed dial as mentioned earlier by @mueller-ma . The problem is, it seems that there is no official APIs to use directly. And it will take at least a thousand lines to build it from scratch to make the speed dial work.

If no beacon name available, it can also display beacon's mac address.

What's more, as @mueller-ma suggested, it can also prompt "Not in range" if a certain paired beacon is not detected.

And for the code of this activity, you can go to OpenHABBeaconActivity and OpenHABBeaconAdapter for more detail. But I'm planning to make this activity looks better by changing it to the layout like the AboutActivity.

@maniac103
Copy link
Contributor

For the speed dial FAB, see https://stackoverflow.com/questions/30699302/android-design-support-library-expandable-floating-action-buttonfab-menu/31422678 ... it both mentions some 3rd party libraries as well as a moderately simple solution for it without 3rd party lib.

@mueller-ma
Copy link
Member

The title should be something like "Location tracking". If the mac is displayed when no name is present, it should have the same visual look.
Apart from that great work!

At some point we can also add location tracking via wifi.

@CasualTriangle
Copy link
Author

looks good so far :)
Do you already have ideas for the "not-debugging interface" design of the different items (beacons, geofences and maybe wifi tracking etc.) of the list ?
like what information is actually relevant for the user there ?
Of the top of my head i would put

  • a simple icon (to differentiate between the different tracking options) (also to avoid a plane text list)
  • a name / something to identify different items (beacon name, geo-coordiantes or ssid)
  • and a status text (in range / inside / outside)

... does that make sense ?

@Tony080
Copy link

Tony080 commented Jun 14, 2018

it both mentions some 3rd party libraries as well as a moderately simple solution for it without 3rd party lib.

So, it's allowed to introduce with 3-rd party libraries, right?

The title should be something like "Location tracking". If the mac is displayed when no name is present, it should have the same visual look.

Yes, it definitely makes sense. And that's why, you know, I claimed that it's an ugly debugging view.

a simple icon

Yes, that could be looked nice.

and a status text (in range / inside / outside)

I'm not sure what's the difference between in range and inside?

@maniac103
Copy link
Contributor

So, it's allowed to introduce with 3-rd party libraries, right?

Yes, sure, it's fine to add them in case they're needed :-)

@Tony080
Copy link

Tony080 commented Jun 14, 2018

I just adjust the GUI's look, and it's refreshing dynamically.

@kaikreuzer
Copy link
Member

Content wise it is a good start, but sorry, the UI still looks like a debugging interface. So yes, a FAB as @mueller-ma suggested above would be nice, but additionally, the list on the screen should also use a design like the one shown here, i.e. similar to a mail inbox (with a round large coloured icon, proper padding and so on).

@maniac103
Copy link
Contributor

similar to a mail inbox (with a round large coloured icon, proper padding and so on).

I fully agree that padding is needed, but the icon(s) IMHO shouldn't be colored, but be monochrome (match primary or secondary text color).

Additionally the FAB also needs padding and a proper (white) icon.

@kaikreuzer
Copy link
Member

the icon(s) IMHO shouldn't be colored

What I meant is "having a color", i.e. not being b/w. - just like the standard material design as in the linked example.

@maniac103
Copy link
Contributor

What I meant is "having a color", i.e. not being b/w. - just like the standard material design as in the linked example.

I got that part, but that's what I specifically not would want to see. Unlike an email app, the icons in our list only depict a very limited set of 'sources' (geofence, beacon, wifi and that's about it?), so in our case we don't need color to differentiate between the items. What I'm thinking about is something similar to this screen from another project of mine (https://github.com/slapperwan/gh4a/):

screenshot_20180615-131541

@Tony080
Copy link

Tony080 commented Jun 16, 2018

Content wise it is a good start, but sorry, the UI still looks like a debugging interface

That's true. Basically, I'm keeping the UI to improve. But it still takes time to make the UI better and better.

the list on the screen should also use a design like the one shown here, i.e. similar to a mail inbox

That a good design. And I think it's quite similar to the UI below.

What I'm thinking about is something similar to this screen from another project of mine (https://github.com/slapperwan/gh4a/):

I downloaded and used the app from my phone. Definitely, that's a good app with beautiful items UI which I should learn from. And it's similar to the UI Kai posted. Thanks for offering the link!

@Tony080
Copy link

Tony080 commented Jun 19, 2018

Update the UI as suggested. Hope this time would be better.

@mueller-ma
Copy link
Member

Looks good, but there is still place for some improvements:
The icons should be grey rather than black, the beacon titles ("BlueCharm") shoudn't be bold, the seperator are too thick and the fab has no padding (See https://material.io/design/components/buttons-floating-action-button.html#specs)

@Tony080
Copy link

Tony080 commented Jun 19, 2018

Looks good, but there is still place for some improvements:

As you suggested, I just changed the UI:

@gitMiguel
Copy link

Any news here?

@cinemarene
Copy link

I would like to ask you to leave a short comment here if this function will ever be developed further.

@mueller-ma
Copy link
Member

There's an open feature request for that (#343), so I'd never say never. Currently I'm not planing to implement this feature, though.

@cinemarene
Copy link

Thank you, I know all the feature requests, and have also read through almost all of them. But what I still don't understand is what is the blocker that prevents it from going further?

@maniac103
Copy link
Contributor

maniac103 commented Jun 9, 2021

the blocker that prevents it from going further?

For starters, the list of things listed under 'not implemented yet' in the PR description + code review comments + adaption to the codebase changes in the meantime (Kotlin conversion, among other things). In other words, it would need somebody to take what's in this PR, rewrite it to Kotlin and add the missing pieces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants